Revisited AST transformers for Kolasu 1.7 #429
Conversation
…rom AST Transformers
…ts for performance
…ransformers really stateless) and
core/src/main/kotlin/com/strumenta/starlasu/mapping/ParseTreeToASTTransformer.kt
Outdated
Show resolved
Hide resolved
ftomassetti
left a comment
ftomassetti
left a comment
There was a problem hiding this comment.
Looks good. I have mostly cosmetic, organizational or naming comments
| val source: Source? = null, | ||
| throwOnUnmappedNode: Boolean = true, | ||
| ) : ASTTransformer(issues, allowGenericNode, throwOnUnmappedNode) { | ||
| faultTolerance: FaultTolerance = FaultTolerance.THROW_ONLY_ON_UNMAPPED, |
| cause: Throwable? = null, | ||
| ) : Exception(message, cause) | ||
|
|
||
| enum class ChildrenPolicy { |
There was a problem hiding this comment.
Nice!
We may also add some javadocs for the entries
core/src/main/kotlin/com/strumenta/starlasu/transformation/Transformation.kt
Outdated
Show resolved
Hide resolved
| var finalizer: (Output) -> Unit = {}, | ||
| var skipChildren: Boolean = false, | ||
| var childrenSetAtConstruction: Boolean = false, | ||
| class Transform<Source, Output : ASTNode>( |
There was a problem hiding this comment.
If TransformationContext is open, should all classes using it be parametric and takes a type parameter TC extending TransformationContext?
There was a problem hiding this comment.
I am not sure about the name. The comment says it is a factory, should it be called one of these?
- Transformation
- Transformer
- TransformationFactory
- TransformerFactory
- NodeFactory
There was a problem hiding this comment.
If
TransformationContextis open, should all classes using it be parametric and takes a type parameter TC extending TransformationContext?
I considered that but I didn't want to further complicate already complex/verbose type signatures. Interested parties can cast a context to the desired type.
I am not sure about the name. The comment says it is a factory, should it be called one of these?
It used to be called NodeFactory, that's why the comment still refers to it as a factory. What about TransformationRule?
There was a problem hiding this comment.
I considered that but I didn't want to further complicate already complex/verbose type signatures. Interested parties can cast a context to the desired type.
Makes sense
It used to be called NodeFactory, that's why the comment still refers to it as a factory. What about TransformationRule?
Yes, I think TransformationRule is a good name
core/src/main/kotlin/com/strumenta/starlasu/mapping/ParseTreeToASTTransformer.kt
Show resolved
Hide resolved
| childType: KClass<out ASTNode> = ASTNode::class, | ||
| ): NodeFactory<Source, Output> { | ||
| ): Transform<Source, Output> { | ||
| if (childrenPolicy == ChildrenPolicy.SKIP) { |
There was a problem hiding this comment.
Should we handle the different children policy through a top-level when statement?
This PR contains a number of changes to AST transformers as listed in #417
Many of these are breaking changes that we couldn't introduce in a patch version update for Kolasu 1.5.